The OnTxStart may need to be called first, then it can use the right env.

Sorry not sure I fully understand the comment.

I mean the onTxStart need be called before OnSystemTxStart.

if tracer := vmConfig.Tracer; tracer != nil {
		if tracer.OnTxStart != nil {
			tracer.OnTxStart(vmenv.GetVMContext(), expectedTx, msg.From())
		}
		if vmConfig.Tracer.OnSystemTxStart != nil {
			vmConfig.Tracer.OnSystemTxStart()
		}
...
}

Because the EVM env only transfers in OnTxStart, so it is better to call first. Do I understand correctly?

func (l *jsonLogger) OnTxStart(env *tracing.VMContext, tx *types.Transaction, from common.Address) {
	l.env = env
}
func (l *jsonLogger) OnSystemTxStart() {
	l.encoder.Encode(l.env.Coinbase, "trigger system tx")
}

I see the OnSystemTxStart acting as a marker. Usually you track the overall state of execution, so I would see an implementation like this:

type Tracer struct {
    (... implement Hooks)
    
    inSystemTrx bool
}

func (t *Tracer) OnSystemTrxStart() { t.inSystemTrx = true }
func (t *Tracer) OnTrxStart() { <track trx normally> }
func (t *Tracer) OnTrxEnd() { if (t.inSystemTrx) { // In sytsem } else { // In normal }  }
func (t *Tracer) OnSystemTrxEnd() { t.inSystemTrx = false }

In that setup, the system trx start must be called first.

Ok, but the defer order is wrong, may

if vmConfig.Tracer.OnSystemTxEnd != nil {
	defer func() {
		vmConfig.Tracer.OnSystemTxEnd()
	}()
}
if tracer.OnTxEnd != nil {
	defer func() {
		tracer.OnTxEnd(tracingReceipt, applyErr)
	}()
}

It will run as you expect:

func (t *Tracer) OnSystemTrxStart() { t.inSystemTrx = true }
func (t *Tracer) OnTrStart() { <track trx normally> }
func (t *Tracer) OnTxEnd() { if (t.inSystemTrx) { // In sytsem } else { // In normal }  }
func (t *Tracer) OnSystemTrxEnd() { t.inSystemTrx = false }

You are so right, good catch, thanks!

Is there Go tests in the project that exercices the parlia consensus engine and test system transactions that I could tweak to ensure the system tracing is fully covered?

you can refer newTestParliaHandlerAfterCancun in eth/handler_test.go:256, there are some integrated testings with parlia. It may help you.

this one change the code logic, the vmConfig was empty before, but now it will use the vmConfig passed from uplayer.

vmenv := vm.NewEVM(context, vm.TxContext{Origin: msg.From(), GasPrice: big.NewInt(0)}, state, chainConfig, vm.Config{})

Should be careful.

That's true.

I can pass the tracer around instead of a pre-built vm.Config instance and I could change back to vm.Config{Tracer: tracer}. Would you prefer that?

@zzzckck

@maoueh I would approve this PR first, and I prefer pass the tracer and change back to vm.Config{Tracer: tracer}, could you create another PR for this change?
I would approve and merge this one first, as it is already quite a while and a big PR.

Yeah sure doing this

Done see #2846